fix: period_config table prefix key schema check #17265

WesselAtWork · 2025-04-16T14:42:43Z

What this PR does / why we need it:

Adds tests and a schema check to prevent a user from setting the prefix key on the period_config to a value containing a path delimiter

Which issue(s) this PR fixes:
Fixes #17232

Special notes for your reviewer:

I updated this buildCache block to be more readable to myself, if you don't like my change, I can revert with no functionality loss

loki/pkg/storage/stores/shipper/indexshipper/storage/cached_client.go

Lines 300 to 320 in 6818c90

    
           for _, object := range objects { 
        
           	// The s3 client can also return the directory itself in the ListObjects. 
        
           	if object.Key == "" { 
        
           		continue 
        
           	} 
        
           	ss := strings.Split(object.Key, delimiter) 
        
           	if len(ss) < 2 || len(ss) > 3 { 
        
           		return fmt.Errorf("invalid key: %s", object.Key) 
        
           	} 
        
           	if len(ss) == 2 { 
        
           		t.commonObjects = append(t.commonObjects, object) 
        
           	} else { 
        
           		userID := ss[1] 
        
           		if len(t.userObjects[userID]) == 0 { 
        
           			t.userIDs = append(t.userIDs, client.StorageCommonPrefix(path.Join(t.name, userID))) 
        
           		} 
        
           		t.userObjects[userID] = append(t.userObjects[userID], object) 
        
           	} 
        
           }

Other:

I could only find examples using some_prefix_term_ in the schema_config_test.go file and could not determine if it is intentional that this field contain a path delimiter, the cache client won't be able to deal with it either way.
This will effect the chunks.prefix as well. If this is intended to contain a delimiter, let me know, I can move the check to the IndexPeriodicTableConfig which should not effect the chunks.prefix
Should I update the documentation as well? Will it be auto generated or do I need to update it there too?
should I update the upgrade doc for this?

Checklist

Reviewed the CONTRIBUTING.md guide (required)
Documentation added
Tests updated
Title matches the required conventional commits format, see here
- Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

CLAassistant · 2025-04-16T14:43:10Z

All committers have signed the CLA.

WesselAtWork · 2025-04-16T14:44:18Z

pkg/storage/stores/shipper/indexshipper/storage/cached_client_test.go

+			path_prefix + "table3/user2/db.gz",
+		}
+		objectClient := newMockObjectClient(t, objectsInStorage)
+		prefixedClient := client.NewPrefixedObjectClient(objectClient, path_prefix)


I used path_prefix here because all the references to NewPrefixedObjectClient seemed to use the PathPrefix from the configuration.
These tests should match the usage more closly now

WesselAtWork · 2025-04-16T14:46:41Z

pkg/storage/stores/shipper/indexshipper/storage/cached_client.go

+		}
+
+		// table/userid/db.gz
+		if len(ss) < 4 {
 			userID := ss[1]
 			if len(t.userObjects[userID]) == 0 {
 				t.userIDs = append(t.userIDs, client.StorageCommonPrefix(path.Join(t.name, userID)))


should this use path.Join?
This might be doing some golang-fu here, but I think this should use t.name + delimiter + userID or something like that?

table prefix key schema check and test

57ab804

WesselAtWork requested a review from a team as a code owner April 16, 2025 14:42

pull-request-size bot added the size/L label Apr 16, 2025

WesselAtWork commented Apr 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: period_config table prefix key schema check #17265

fix: period_config table prefix key schema check #17265

WesselAtWork commented Apr 16, 2025

CLAassistant commented Apr 16, 2025 •

edited

Loading

WesselAtWork Apr 16, 2025

WesselAtWork Apr 16, 2025

	for _, object := range objects {
	// The s3 client can also return the directory itself in the ListObjects.
	if object.Key == "" {
	continue
	}

	ss := strings.Split(object.Key, delimiter)
	if len(ss) < 2 \|\| len(ss) > 3 {
	return fmt.Errorf("invalid key: %s", object.Key)
	}

	if len(ss) == 2 {
	t.commonObjects = append(t.commonObjects, object)
	} else {
	userID := ss[1]
	if len(t.userObjects[userID]) == 0 {
	t.userIDs = append(t.userIDs, client.StorageCommonPrefix(path.Join(t.name, userID)))
	}
	t.userObjects[userID] = append(t.userObjects[userID], object)
	}
	}

fix: period_config table prefix key schema check #17265

Are you sure you want to change the base?

fix: period_config table prefix key schema check #17265

Conversation

WesselAtWork commented Apr 16, 2025

CLAassistant commented Apr 16, 2025 • edited Loading

WesselAtWork Apr 16, 2025

Choose a reason for hiding this comment

WesselAtWork Apr 16, 2025

Choose a reason for hiding this comment

CLAassistant commented Apr 16, 2025 •

edited

Loading